matthewwithanm / pilkit

Utilities and processors built for, and on top of PIL
BSD 3-Clause "New" or "Revised" License
196 stars 54 forks source link

fix issue #24 #25

Closed hassanabidpk closed 7 years ago

hassanabidpk commented 7 years ago

Fixed this issue by adding try and except block. I tested it on my Django site which is deployed on Azure.

vstoykov commented 7 years ago

Hello @hassanabidpk. Thanks for your contribution. Now when I see the logic there I think that the way you solved the issue is too rough.

Can you try to modify it to take the approach for /dev/nul (https://github.com/matthewwithanm/pilkit/commit/07748f906833a774cffbea8ff7330ebe3007d288)

hassanabidpk commented 7 years ago

Hi @vstoykov . Thanks for your feedback. Do you mean something like this ?

try:
        self.stderr_fd = sys.__stderr__.fileno()
except AttributeError:
       # In case of Azure, file descriptor is not present so we can return from here
        return
self.old = os.dup(self.stderr_fd)
os.dup2(self.null_fd, self.stderr_fd)

Actually, I have tested OSError but it doesn't seem to work

vstoykov commented 7 years ago

Yes something like that.

Also at __exit__, check if self.stderr_fd is present the same way as for self.null_fd. Or probably better, at __exit__ only presence of self.old need to be checked because if we return early in __enter__ no mater in which case self.old will not be set.

hassanabidpk commented 7 years ago

Okay, I updated according to your suggested and also tested on my azure server.

vstoykov commented 7 years ago

Now when I'm looking at the final result I see some problems that you need to address:

  1. the logic in the if in __exit__ is not correct
  2. There is no need os.devnull to be opened if sys.__stderr__ does not have fileno or we risk to leave open file.

I know that it is more code but is better to try-catch only for what you want to catch. I'm talking of two separate try-catch blocks for both possible exceptions.

hassanabidpk commented 7 years ago

Alright Thanks for your suggestion @vstoykov . I will do a clean pull request as this one got more commits 👍